Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Fix IOperation for asynchronous using and foreach statements #37963

Merged
merged 7 commits into from
Aug 23, 2019
Merged

Fix IOperation for asynchronous using and foreach statements #37963

merged 7 commits into from
Aug 23, 2019

Conversation

jcouv
Copy link
Member

@jcouv jcouv commented Aug 13, 2019

Fixes #30362

  • add IsAsynchronous to IUsingOperation and IForEachLoopOperation
  • add IsAsynchronous to ForeachEnumeratorInfo
  • add missing awaits on invocations of DisposeAsync() and MoveNextAsync()
  • correct references to IDisposable to be IAsyncDisposable

@jcouv jcouv added this to the 16.4 milestone Aug 13, 2019
@jcouv jcouv self-assigned this Aug 13, 2019
@jcouv jcouv marked this pull request as ready for review August 13, 2019 23:10
@jcouv jcouv requested a review from a team as a code owner August 13, 2019 23:10
@jcouv
Copy link
Member Author

jcouv commented Aug 14, 2019 •
edited
Loading

@AlekseyTs I thought I'd have something more to do for CFG, but I think I've already handled it. Thanks #Resolved

@jcouv
Copy link
Member Author

jcouv commented Aug 20, 2019 •
edited by 333fred
Loading

@333fred for review. Thanks #Closed

333fred
333fred reviewed Aug 21, 2019
View reviewed changes
src/Compilers/CSharp/Test/IOperation/IOperation/IOperationTests_IForEachLoopStatement.cs
Locals: Local_1: System.String value
LoopControlVariable:
IVariableDeclaratorOperation (Symbol: System.String value) (OperationKind.VariableDeclarator, Type: null) (Syntax: 'string')
Initializer:
null
Collection:
IParameterReferenceOperation: pets (OperationKind.ParameterReference, Type: System.Collections.Generic.IAsyncEnumerable<System.String>) (Syntax: 'pets')
IConversionOperation (TryCast: False, Unchecked) (OperationKind.Conversion, Type: System.Collections.Generic.IAsyncEnumerable<System.String>, IsImplicit) (Syntax: 'pets')
Copy link
Member

@333fred 333fred Aug 21, 2019 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do you know why this conversion is introduced? #Closed

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not sure. The logic in BindForEachPartsWorker always wraps the collection expression with a BoundConversion (even when the conversion is Identity).
The conversion is there in other IOperation tests for regular foreach loops (ForEachFlow_06) so this seems correct.

Let me know if this bugs you. I can investigate further.

In reply to: 316309905 [](ancestors = 316309905)

@333fred
Copy link
Member

333fred commented Aug 21, 2019 •
edited
Loading

Please add some tests with the IAsyncX interfaces missing in the compilation for both IOp and CFG generation. #Closed

333fred
333fred reviewed Aug 21, 2019
View reviewed changes
src/Test/Utilities/Portable/Compilation/OperationTreeVerifier.cs Outdated
@@ -600,6 +600,10 @@ private void LogLoopStatementHeader(ILoopOperation operation, bool? isChecked =
var propertyStringBuilder = new StringBuilder();
propertyStringBuilder.Append(" (");
propertyStringBuilder.Append($"{nameof(LoopKind)}.{operation.LoopKind}");
if (operation is IForEachLoopOperation foreachLoop && foreachLoop.IsAsynchronous)
Copy link
Member

@333fred 333fred Aug 21, 2019 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

foreachLoop && foreachLoop.IsAsynchronous [](start = 51, length = 41)

Nit: could use recursive pattern :) #Closed

@333fred
Copy link
Member

333fred commented Aug 21, 2019 •
edited
Loading

Done review pass (commit 4) #Closed

333fred
333fred approved these changes Aug 22, 2019
View reviewed changes
Copy link
Member

@333fred 333fred left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM (commit 5)

@jcouv
Copy link
Member Author

jcouv commented Aug 22, 2019

@dotnet/roslyn-compiler for another review.
Thanks @333fred!

cston
cston reviewed Aug 22, 2019
View reviewed changes
src/Compilers/Core/Portable/Operations/OperationInterfaces.xml Outdated
<Property Name="IsAsynchronous" Type="bool">
<Comments>
<summary>
Whether this for using is asynchronous.
Copy link
Member

@cston cston Aug 22, 2019 •
edited by jcouv
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

for [](start = 23, length = 3)

Remove? #Pending

cston
cston reviewed Aug 22, 2019
View reviewed changes
src/Compilers/Core/Portable/Operations/ControlFlowGraphBuilder.cs Outdated
@@ -4110,9 +4122,15 @@ public override IOperation VisitForEachLoop(IForEachLoopOperation operation, int

LeaveRegion();

bool isAsynchronous = info.IsAsynchronous;
var iDisposable = isAsynchronous
? _compilation.GetTypeByMetadataName(WellKnownTypes.GetMetadataName(WellKnownType.System_IAsyncDisposable))
Copy link
Member

@cston cston Aug 22, 2019 •
edited by jcouv
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

_compilation.GetTypeByMetadataName(WellKnownTypes.GetMetadataName(WellKnownType.System_IAsyncDisposable) [](start = 22, length = 104)

Should there be a CommonGetWellKnownType() method? #Resolved

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should use new CommonGetWellKnownType() method.

In reply to: 316817457 [](ancestors = 316817457)

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oops. Thanks

In reply to: 316915353 [](ancestors = 316915353,316817457)

cston
cston reviewed Aug 22, 2019
View reviewed changes
src/Compilers/CSharp/Portable/Binder/ForEachEnumeratorInfo.cs Outdated
@@ -30,6 +30,8 @@ internal sealed class ForEachEnumeratorInfo
// When async and needs disposal, this stores the information to await the DisposeAsync() invocation
public AwaitableInfo DisposeAwaitableInfo;

public readonly bool IsAsync;
Copy link
Member

@cston cston Aug 22, 2019 •
edited by jcouv
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

bool [](start = 24, length = 4)

Minor point: Perhaps move next to NeedsDisposal to reduce size of instance. #Resolved

cston
cston reviewed Aug 22, 2019
View reviewed changes
src/Compilers/Core/Portable/Operations/ControlFlowGraphBuilder.cs Outdated
@@ -3841,7 +3852,8 @@ bool isNotNullableValueType(ITypeSymbol type)

private IOperation ConvertToIDisposable(IOperation operand, ITypeSymbol iDisposable, bool isTryCast = false)
{
Debug.Assert(iDisposable.SpecialType == SpecialType.System_IDisposable);
Debug.Assert(iDisposable.SpecialType == SpecialType.System_IDisposable ||
iDisposable.Equals( _compilation.GetTypeByMetadataName(WellKnownTypes.GetMetadataName(WellKnownType.System_IAsyncDisposable))));
Copy link
Member

@cston cston Aug 22, 2019 •
edited by jcouv
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

GetTypeByMetadataName [](start = 49, length = 21)

CommonGetWellKnownType() #Resolved

@jcouv jcouv changed the base branch from release/dev16.4-preview1 to master August 23, 2019 18:59
@jcouv jcouv merged commit 036d931 into dotnet:master Aug 23, 2019
@jcouv jcouv deleted the await-ioperation branch August 23, 2019 21:32
@jcouv jcouv modified the milestones: 16.4, 16.4.P1 Sep 6, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@333fred 333fred 333fred approved these changes

@cston cston cston approved these changes

@chsienki chsienki chsienki approved these changes

Assignees

@jcouv jcouv

Labels
Area-Compilers New Feature - Async Streams Async Streams
Projects
None yet
Milestone
16.4.P1
Development

Successfully merging this pull request may close these issues.

Finalize design and implementation of IOperation and CFG for async-streams feature
4 participants
@jcouv @333fred @cston @chsienki